Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OAuth2 Authorization Code Grant Flow #247

Merged
merged 79 commits into from
Jun 25, 2021
Merged

OAuth2 Authorization Code Grant Flow #247

merged 79 commits into from
Jun 25, 2021

Conversation

johakoch
Copy link
Collaborator

No description provided.

@johakoch johakoch force-pushed the oauth2-ac branch 9 times, most recently from 13c73b0 to 1a02ddb Compare May 28, 2021 07:34
@johakoch johakoch marked this pull request as ready for review June 11, 2021 12:54
@johakoch
Copy link
Collaborator Author

Im Moment ist es so implementiert, dass erwartet wird, dass Code-Verifier oder CSRF-Token als solche an den Browser gehen: Im Request an den Redirekt-Endpunkt müssen sie aus dem Request ermittelbar sein (z.B. im Cookie). Dafür gibt es die Konfigurations-Attribute code_verifier_value und csrf_token_value:

oauth2 "my-ac" {
  ...
  code_verifier_value = request.cookies.cv
#  csrf_token_value = request.cookies.ct
  ...
}

Ohne einen zentralen Store ist es dann auch wahrscheinlich, dass Access-/Id-/Refresh-Tokens als solche (als Cookie) im Browser gespeichert werden. Hätte man einen (von verschiedenen Couper-Instanzen verwendeten) zentralen Store, könnte man die Tokens selbst im Store speichern und nur eine Referenz darauf an den Browser gehen. Das würde ggfs. die Sicherheit noch erhöhen.
Ebenso könnten dann Code-Verifier/CSRF-Token selbst im Store verbleiben und nur Referenzen darauf an den Browser gehen.
Verbaut man sich nun durch die genannten Konfigurations-Attribute eine mögliche Erweiterung durch einen zentralen Store? Vermutlich nicht: code_verifier_value/csrf_token_value müssten dann nur anders interpretiert werden, nämlich als Referenzen auf die eigentlichen Werte. Welche Variante gewünscht ist, ließe sich sicherlich konfigurativ ausdrücken.

@filex
Copy link
Contributor

filex commented Jun 16, 2021

Verbaut man sich nun durch die genannten Konfigurations-Attribute eine mögliche Erweiterung durch einen zentralen Store? Vermutlich nicht: code_verifier_value/csrf_token_value müssten dann nur anders interpretiert werden, nämlich als Referenzen auf die eigentlichen Werte. Welche Variante gewünscht ist, ließe sich sicherlich konfigurativ ausdrücken.

The current idea is, that code_verifier_value is defined as an arbitrary expression that usually involves cookies. if we would incorporate a KV interface, we could provide a lookup function as a simple solution. That would still fit the expression approach.

But I would rather make that token store a built-in feature that is configured outside of the AC. We could then reference it with a new set of config attributes that are mutually exclusive with code_verifier_value.

For the task here, I agree that we can proceed with the value-approach.

Copy link
Collaborator

@malud malud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First go related feedback.

accesscontrol/jwks.go Outdated Show resolved Hide resolved
accesscontrol/jwks.go Outdated Show resolved Hide resolved
accesscontrol/oauth2.go Outdated Show resolved Hide resolved
// Validate implements the AccessControl interface
func (oa *OAuth2Callback) Validate(req *http.Request) error {
if req.Method != http.MethodGet {
return errors.Oauth2.Messagef("wrong method: %s", req.Method)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This applies to all Oauth2 errors, could we add more context e.g. oa.conf.Name as .Label(...) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the caller give the context? Or the callee?

accesscontrol/oauth2.go Outdated Show resolved Hide resolved
docs/README.md Outdated
| `csrf` | <ul><li>&#9888; one of `pkce` or `csrf` is mandatory.</li><li>Use `state` or `nonce` for protection against CSRF.</li></ul> |
| **Attributes** | **Description** |
| `authorization_endpoint` | <ul><li>&#9888; Mandatory.</li><li>The authorization server endpoint URL used for authorization.</li></ul> |
| `token_endpoint` | <ul><li>Optional.</li><li>The authorization server endpoint URL used for requesting the token.</li></ul> |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation needs to be rebased but this table should get another column for required/optional instead of html listings. Other tables will be adapted soon.

eval/lib/oauth2.go Outdated Show resolved Hide resolved
eval/lib/oauth2.go Outdated Show resolved Hide resolved
handler/transport/oauth2.go Outdated Show resolved Hide resolved
utils/base64url/base64url.go Outdated Show resolved Hide resolved
eval/context.go Outdated Show resolved Hide resolved
Johannes Koch added 2 commits June 23, 2021 13:47
…rant flow"

Reintroduced userinfo request for OIDC conformance tests.

This reverts commit 0d1c4e7.
Copy link
Collaborator

@malud malud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some things to do, please see the comments from last week.

handler/transport/oauth2_req_auth.go Outdated Show resolved Hide resolved
server/http_oauth2_test.go Show resolved Hide resolved
server/http_oauth2_test.go Outdated Show resolved Hide resolved
Johannes Koch added 3 commits June 25, 2021 15:47
Solving Conflicts:
*	CHANGELOG.md
*	docs/REFERENCE.md
*	eval/context.go
*	eval/lib/jwt.go

and adding more documentation
@johakoch johakoch merged commit 0dd0d44 into master Jun 25, 2021
@johakoch johakoch deleted the oauth2-ac branch June 25, 2021 17:13
malud pushed a commit that referenced this pull request Aug 12, 2021
malud pushed a commit that referenced this pull request Aug 13, 2021
malud pushed a commit that referenced this pull request Aug 17, 2021
johakoch pushed a commit that referenced this pull request Aug 19, 2021
johakoch pushed a commit that referenced this pull request Aug 19, 2021
johakoch pushed a commit that referenced this pull request Aug 20, 2021
* Fix oauth_authorization_url function call to undefined reference

* Fix missing cache setup on conf dry run

* Fix missing name log value

* oidc block ttl attribute is optional now; with 1h default

* Change: oauth2/oidc redirect_uri attribute gets evaluated per request now

* Add shutdown logAll hook entries for failed tests

* Remove obsolete method #247

* Add oidc eval test

* fixup obsolete assignment

* Fixup possible startup errors while fetching the oidc configuration

Pass uid
Fixed missing update of the jwt parser if the issuer has been changed

* Change memCache set/get to interface{} value

* Fixup missing lock on oidc configuration refresh

* Renamed CallbackURL -> RedirectURI

* Ensure that redirectURI is not empty

* Handling of undefinied reference in saml_sso_url() similar to beta_oauth_authorization_url()

* Do not load openid configuration at start-up; only GetVerifierMethod() and GetAuthorizationEndpoint() need uid, as they are the first methods calling the openid configuration

* Reference() already in config.OIDC; removed from oidc.Config

* split OAuth clients code into different files

* better error log messages

* removed unused return values

* more information in OAuth2 error messages

* renamed beta_oidc.ttl -> beta_oidc.configuration_ttl

* missing optional configuration_ttl does not crash

Co-authored-by: Johannes Koch <johannes.koch@avenga.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants